fix unloading #71
fix unloading #71
Conversation
WalkthroughReworked ExecuteQueryToFile flow to use async disposal and CSV streaming, updated CsvFileWriter to implement IAsyncDisposable and renamed SaveQueryToCsv, bumped project version to 2.2.0 and added changelog entry; minor whitespace changes in enum files. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ExecuteQueryToFile
participant SQLServer
participant CsvFileWriter
participant FileSystem
Caller ->> ExecuteQueryToFile: Invoke ExecuteQueryToFile(params, options, ct)
ExecuteQueryToFile ->> SQLServer: OpenAsync connection
ExecuteQueryToFile ->> SQLServer: Execute command with parameters (ExecuteReaderAsync)
SQLServer -->> ExecuteQueryToFile: DbDataReader (streaming rows)
ExecuteQueryToFile ->> CsvFileWriter: Create CsvFileWriter(resultPath, options)
ExecuteQueryToFile ->> CsvFileWriter: SaveQueryToCsv(cancellationToken)
CsvFileWriter ->> CsvFileWriter: Async-write headers and rows
CsvFileWriter ->> FileSystem: FlushAsync & close file
CsvFileWriter -->> ExecuteQueryToFile: Result (path, metadata)
ExecuteQueryToFile -->> Caller: Return Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.cs`:
- Line 38: The Result class for ExecuteQueryToFile is missing the required
Success bool and Error object; update the ExecuteQueryToFile.Result type to
include a public bool Success property and an Error property (object/class) with
Message and AdditionalInfo fields, while keeping existing task-specific
properties (EntriesWritten, Path, FileName); mirror the shape used by
ExecuteQuery.Result (or reuse its Error type) so callers can check Success and
read Error.Message/Error.AdditionalInfo when failures occur.
In
`@Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.csproj`:
- Line 6: The Result class in Definitions/Result.cs is missing the public
properties referenced by the pipeline; add public bool Success { get; set; } and
public string Error { get; set; } to the Result class (ensure they are public
auto-properties on the Result type so existing callers/serialization continue to
work).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Frends.MicrosoftSQL.ExecuteQueryToFile/CHANGELOG.mdFrends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.csFrends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.csproj
🧰 Additional context used
📓 Path-based instructions (4)
Frends.*/CHANGELOG.md
⚙️ CodeRabbit configuration file
Frends.*/CHANGELOG.md: Validate format against Keep a Changelog (https://keepachangelog.com/en/1.0.0/)
Include all functional changes and indicate breaking changes with upgrade notes.
Avoid notes like "refactored xyz" unless it affects functionality.
Files:
Frends.MicrosoftSQL.ExecuteQueryToFile/CHANGELOG.md
Frends.*/Frends.*/*.cs
⚙️ CodeRabbit configuration file
Frends.*/Frends.*/*.cs: Ensure every public method and class:
- Has
and XML comments
- If the documentation is very long then it can also use element
- Follows Microsoft C# code conventions
- Uses semantic task result documentation (e.g., Success, Error, Data)
Frends.*/Frends.*/*.cs: Validate all task result classes include:
- Success (bool)
- Task-specific return value (e.g. Data, FilePaths)
- Error object with Message and AdditionalInfo
- Ensure result structure is flat, simple, and avoids 3rd-party types.
- Use dynamic or JToken only when the structure is unknown.
Files:
Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.cs
Frends.*/**/*.cs
⚙️ CodeRabbit configuration file
Frends.*/**/*.cs: Code must follow Microsoft C# coding standards, including:
- PascalCase for public members and task parameters
- Proper naming for abbreviations (Csv, Url, Api)
- Use of var only when type is obvious
- Clean structure and no unused code
Files:
Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.cs
Frends.*/Frends.*/*.csproj
⚙️ CodeRabbit configuration file
Frends.*/Frends.*/*.csproj: Ensure the .csproj targets .NET 6 or 8, uses the MIT license, and includes the following fields:
= Frends
= true
= MIT
Follow Microsoft C# project file conventions.
Files:
Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.csproj
🪛 GitHub Actions: ExecuteQueryToFile_build_test
Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.cs
[error] 38-38: FT0011: Class should include a 'Success' property
[error] 38-38: FT0011: Class should include a 'Error' property
Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.csproj
[error] 38-38: FT0011: Class should include a 'Success' property
[error] 38-38: FT0011: Class should include a 'Error' property
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
Frends.MicrosoftSQL.ExecuteQueryToFile/CHANGELOG.md (1)
3-5: LGTM!The changelog entry follows the Keep a Changelog format correctly with proper versioning, date format, and categorization under "Fixed".
Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.cs (2)
19-28: Static constructor pattern for assembly unload subscription looks correct.The null check on
currentContextis appropriate, and subscribing toUnloadingin a static constructor is a valid pattern for plugin lifecycle management.
84-87: Verify the unload handler implementation is complete.The handler only unsubscribes itself from the
Unloadingevent but performs no actual resource cleanup. If the goal is to "fix unloading assembly" issues, this handler may be missing cleanup logic (e.g., disposing connections, clearing caches, releasing native resources).Is this intentional? If the fix requires only removing the event subscription to prevent the assembly from being held in memory, this is sufficient. However, if there are other resources that need cleanup during unload, they should be added here.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...QueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.cs
Outdated
Show resolved
Hide resolved
...yToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.csproj
Show resolved
Hide resolved
jefim
left a comment
jefim
left a comment
There was a problem hiding this comment.
So, I see that you have an error handler for unloading, but it does nothing? Why?
Well yes - I messed up with this one 😅 I've started with this template, then must have switch to smth else and when I came back "I saw PR created, tests passed - let's go!" instead of actual finish a fix. sorry for that. Now I've pushed correct changes. I think we don't actual need unloading assemblies mechanism this time |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Definitions/CsvFileWriter.cs`:
- Around line 33-36: The public method DisposeAsync in class CsvFileWriter is
missing XML documentation; add an XML doc comment above the DisposeAsync method
that includes at minimum a <summary> describing that it asynchronously disposes
resources (e.g., disposes SqlCommand if not null) and a <returns> describing the
returned ValueTask; reference the DisposeAsync method and the SqlCommand member
in the comment to make intent clear.
- Around line 38-49: Add a missing XML <summary> for the SaveQueryToCsv method
and ensure the DbDataReader returned by SqlCommand.ExecuteReaderAsync is
disposed: wrap the reader in an await using (or using) block when calling
SqlCommand.ExecuteReaderAsync inside SaveQueryToCsv so reader is disposed after
DataReaderToCsv completes, and update the method's XML documentation to include
a <summary> describing its purpose (SaveQueryToCsv) and any relevant
param/return tags.
♻️ Duplicate comments (1)
Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.cs (1)
25-28: Result class missingSuccessandErrorproperties (pipeline failure).The pipeline error FT0011 indicates the
Resultclass is missing required properties. This was already flagged in a previous review. Per the coding guidelines, all task result classes must include:
Success(bool)Errorobject withMessageandAdditionalInfo
🧹 Nitpick comments (1)
Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile.cs (1)
59-68: Consider adding a default case or exhaustive handling.The switch only handles
ReturnFormat.CSV. If otherReturnFormatvalues exist or are added, they would silently return an uninitializedResult. Consider adding adefaultcase that either throwsNotSupportedExceptionor explicitly handles unsupported formats.♻️ Suggested improvement
switch (options.ReturnFormat) { case ReturnFormat.CSV: { await using var csvWriter = new CsvFileWriter(command, input, options.CsvOptions); result = await csvWriter.SaveQueryToCsv(cancellationToken).ConfigureAwait(false); break; } + default: + throw new NotSupportedException($"Return format '{options.ReturnFormat}' is not supported."); }
| public async ValueTask DisposeAsync() | ||
| { | ||
| var output = 0; | ||
| var encoding = GetEncoding(Options.FileEncoding, Options.EnableBom, Options.EncodingInString); | ||
|
|
||
| using (var writer = new StreamWriter(Input.OutputFilePath, false, encoding)) | ||
| using (var csvFile = CreateCsvWriter(Options.GetFieldDelimiterAsString(), writer)) | ||
| { | ||
| writer.NewLine = Options.GetLineBreakAsString(); | ||
|
|
||
| var reader = await SqlCommand.ExecuteReaderAsync(cancellationToken); | ||
| output = DataReaderToCsv(reader, csvFile, Options, cancellationToken); | ||
| if (SqlCommand != null) await SqlCommand.DisposeAsync(); | ||
| } |
There was a problem hiding this comment.
Add XML documentation to fix pipeline failure.
Pipeline error FT0014 indicates missing <summary> documentation tag on this public method.
📝 Proposed fix
+ /// <summary>
+ /// Disposes the underlying SQL command asynchronously.
+ /// </summary>
public async ValueTask DisposeAsync()
{
if (SqlCommand != null) await SqlCommand.DisposeAsync();
+ GC.SuppressFinalize(this);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async ValueTask DisposeAsync() | |
| { | |
| var output = 0; | |
| var encoding = GetEncoding(Options.FileEncoding, Options.EnableBom, Options.EncodingInString); | |
| using (var writer = new StreamWriter(Input.OutputFilePath, false, encoding)) | |
| using (var csvFile = CreateCsvWriter(Options.GetFieldDelimiterAsString(), writer)) | |
| { | |
| writer.NewLine = Options.GetLineBreakAsString(); | |
| var reader = await SqlCommand.ExecuteReaderAsync(cancellationToken); | |
| output = DataReaderToCsv(reader, csvFile, Options, cancellationToken); | |
| if (SqlCommand != null) await SqlCommand.DisposeAsync(); | |
| } | |
| /// <summary> | |
| /// Disposes the underlying SQL command asynchronously. | |
| /// </summary> | |
| public async ValueTask DisposeAsync() | |
| { | |
| if (SqlCommand != null) await SqlCommand.DisposeAsync(); | |
| GC.SuppressFinalize(this); | |
| } |
🧰 Tools
🪛 GitHub Actions: ExecuteQueryToFile_build_test
[error] 33-33: FT0014: Missing a required documentation tag 'summary'.
🤖 Prompt for AI Agents
In
`@Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Definitions/CsvFileWriter.cs`
around lines 33 - 36, The public method DisposeAsync in class CsvFileWriter is
missing XML documentation; add an XML doc comment above the DisposeAsync method
that includes at minimum a <summary> describing that it asynchronously disposes
resources (e.g., disposes SqlCommand if not null) and a <returns> describing the
returned ValueTask; reference the DisposeAsync method and the SqlCommand member
in the comment to make intent clear.
| public async Task<Result> SaveQueryToCsv(CancellationToken cancellationToken) | ||
| { | ||
| var encoding = GetEncoding(Options.FileEncoding, Options.EnableBom, Options.EncodingInString); | ||
| await using var writer = new StreamWriter(Input.OutputFilePath, false, encoding); | ||
| await using var csvFile = CreateCsvWriter(Options.GetFieldDelimiterAsString(), writer); | ||
| writer.NewLine = Options.GetLineBreakAsString(); | ||
| var reader = await SqlCommand.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false); | ||
| var output = DataReaderToCsv(reader, csvFile, Options, cancellationToken); | ||
| await csvFile.FlushAsync().ConfigureAwait(false); | ||
|
|
||
| return new Result(output, Input.OutputFilePath, Path.GetFileName(Input.OutputFilePath)); | ||
| } |
There was a problem hiding this comment.
Add XML documentation and fix DbDataReader resource leak.
Two issues here:
- Pipeline error FT0014: Missing
<summary>documentation tag. - Critical: The
DbDataReaderfromExecuteReaderAsync(line 44) is never disposed, causing a resource leak that can exhaust the connection pool.
🐛 Proposed fix
+ /// <summary>
+ /// Executes the SQL query and writes the results to a CSV file.
+ /// </summary>
+ /// <param name="cancellationToken">Cancellation token.</param>
+ /// <returns>Result containing entries written and file path information.</returns>
public async Task<Result> SaveQueryToCsv(CancellationToken cancellationToken)
{
var encoding = GetEncoding(Options.FileEncoding, Options.EnableBom, Options.EncodingInString);
await using var writer = new StreamWriter(Input.OutputFilePath, false, encoding);
await using var csvFile = CreateCsvWriter(Options.GetFieldDelimiterAsString(), writer);
writer.NewLine = Options.GetLineBreakAsString();
- var reader = await SqlCommand.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false);
- var output = DataReaderToCsv(reader, csvFile, Options, cancellationToken);
+ await using var reader = await SqlCommand.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false);
+ var output = DataReaderToCsv(reader, csvFile, Options, cancellationToken);
await csvFile.FlushAsync().ConfigureAwait(false);
return new Result(output, Input.OutputFilePath, Path.GetFileName(Input.OutputFilePath));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task<Result> SaveQueryToCsv(CancellationToken cancellationToken) | |
| { | |
| var encoding = GetEncoding(Options.FileEncoding, Options.EnableBom, Options.EncodingInString); | |
| await using var writer = new StreamWriter(Input.OutputFilePath, false, encoding); | |
| await using var csvFile = CreateCsvWriter(Options.GetFieldDelimiterAsString(), writer); | |
| writer.NewLine = Options.GetLineBreakAsString(); | |
| var reader = await SqlCommand.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false); | |
| var output = DataReaderToCsv(reader, csvFile, Options, cancellationToken); | |
| await csvFile.FlushAsync().ConfigureAwait(false); | |
| return new Result(output, Input.OutputFilePath, Path.GetFileName(Input.OutputFilePath)); | |
| } | |
| /// <summary> | |
| /// Executes the SQL query and writes the results to a CSV file. | |
| /// </summary> | |
| /// <param name="cancellationToken">Cancellation token.</param> | |
| /// <returns>Result containing entries written and file path information.</returns> | |
| public async Task<Result> SaveQueryToCsv(CancellationToken cancellationToken) | |
| { | |
| var encoding = GetEncoding(Options.FileEncoding, Options.EnableBom, Options.EncodingInString); | |
| await using var writer = new StreamWriter(Input.OutputFilePath, false, encoding); | |
| await using var csvFile = CreateCsvWriter(Options.GetFieldDelimiterAsString(), writer); | |
| writer.NewLine = Options.GetLineBreakAsString(); | |
| await using var reader = await SqlCommand.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false); | |
| var output = DataReaderToCsv(reader, csvFile, Options, cancellationToken); | |
| await csvFile.FlushAsync().ConfigureAwait(false); | |
| return new Result(output, Input.OutputFilePath, Path.GetFileName(Input.OutputFilePath)); | |
| } |
🧰 Tools
🪛 GitHub Actions: ExecuteQueryToFile_build_test
[error] 38-38: FT0014: Missing a required documentation tag 'summary'.
🤖 Prompt for AI Agents
In
`@Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Definitions/CsvFileWriter.cs`
around lines 38 - 49, Add a missing XML <summary> for the SaveQueryToCsv method
and ensure the DbDataReader returned by SqlCommand.ExecuteReaderAsync is
disposed: wrap the reader in an await using (or using) block when calling
SqlCommand.ExecuteReaderAsync inside SaveQueryToCsv so reader is disposed after
DataReaderToCsv completes, and update the method's XML documentation to include
a <summary> describing its purpose (SaveQueryToCsv) and any relevant
param/return tags.
Review Checklist
Summary by CodeRabbit
Bug Fixes
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.